Skip to content

Conversation

@uncleGen
Copy link
Contributor

What changes were proposed in this pull request?

add rest api for job environment.

How was this patch tested?

existing ut.

@SparkQA
Copy link

SparkQA commented Feb 16, 2017

Test build #72979 has finished for PR 16949 at commit a3d2abb.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@uncleGen
Copy link
Contributor Author

jenkins crushed. retest this please.

@SparkQA
Copy link

SparkQA commented Feb 16, 2017

Test build #72986 has started for PR 16949 at commit a3d2abb.

@uncleGen
Copy link
Contributor Author

cc @srowen

@uncleGen
Copy link
Contributor Author

terminated by signal 9. retest this please.

@SparkQA
Copy link

SparkQA commented Feb 16, 2017

Test build #72989 has finished for PR 16949 at commit a3d2abb.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@srowen
Copy link
Member

srowen commented Feb 16, 2017

It's a simple change, but I wonder if this is that important to add?
I always have a worry in the back of my mind that this becomes a security hole, as it's a way to look through the complete environment of a bunch of jobs.

@uncleGen
Copy link
Contributor Author

uncleGen commented Feb 16, 2017

@srowen good question!IMHO,we should add this API:

  • provide complete API, the same as users see in webui
  • if this is a security issue, we should address it in other ways
  • existing API also has security issue as you said. Maybe, we need some authorization check or something else, just you said security issue.

Any suggestion is appreciated!

Copy link
Member

@ajbozarth ajbozarth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @srowen that this highlights one of the web ui's current security holes, access to the system env via the web ui, but like @uncleGen said, this is already an issue in the current code. I like expanding the REST API to include the same info as the web ui, especially since it will be necessary if @vanzin 's history server project is accepted. Overall LGTM.

@ajbozarth
Copy link
Member

@uncleGen As followup I'd recommend opening a JIRA to research and address the potential security flaws in the env tab. I know we've already merged some fixes to hide passwords in the env tab, but we should make sure there aren't more issues.

@vanzin
Copy link
Contributor

vanzin commented Feb 16, 2017

re: security, this change doesn't really change anything; all this information is already available in the UI. Note it doesn't expose the system env (as in environment variables), although people may still expose things through system properties and spark configuration. There's some code to redact sensitive info and even a config to control what gets redacted, but ultimately, the solution to security is auth + ssl. As Spark gets more and more features, data may start leaking through other places (e.g. SQL query plans which are shown in the UI).

As for the change, I kinda implemented it as part of my SHS project, and I went with a slightly different API:
vanzin@d127f15

Mainly I broke the "JVM properties" into a separate type; I kinda prefer that instead of "magic keys", especially when these keys contain spaces and thus don't map well to code.

@uncleGen
Copy link
Contributor Author

@vanzin I opened a jira (https://issues.apache.org/jira/browse/SPARK-19642) to research and address the potential security flaws. Do you mind if I continue this pr?

@vanzin
Copy link
Contributor

vanzin commented Feb 17, 2017

Sure, this PR is fine, I'd just prefer some minor API adjustments to bring it closer to the code I linked above.

@ajbozarth
Copy link
Member

Thanks @uncleGen and after seeing his code I agree with @vanzin

@uncleGen
Copy link
Contributor Author

uncleGen commented Feb 17, 2017

@vanzin @ajbozarth sure, I will do some update based on the linked code.

@SparkQA
Copy link

SparkQA commented Feb 20, 2017

Test build #73145 has finished for PR 16949 at commit ad570cf.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@ajbozarth
Copy link
Member

LGTM

@uncleGen
Copy link
Contributor Author

cc @srowen and @vanzin also.

Copy link
Contributor

@vanzin vanzin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but needs an update because of other changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to update your diff because this method doesn't exist anymore.

@SparkQA
Copy link

SparkQA commented Feb 23, 2017

Test build #73309 has finished for PR 16949 at commit 6094743.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@vanzin
Copy link
Contributor

vanzin commented Feb 23, 2017

Merging to master.

@asfgit asfgit closed this in 66c4b79 Feb 23, 2017
asfgit pushed a commit that referenced this pull request Feb 24, 2017
## What changes were proposed in this pull request?

follow up pr of #16949.

## How was this patch tested?

jenkins

Author: uncleGen <[email protected]>

Closes #17033 from uncleGen/doc-restapi-environment.
Yunni pushed a commit to Yunni/spark that referenced this pull request Feb 27, 2017
## What changes were proposed in this pull request?

add rest api for job environment.

## How was this patch tested?

existing ut.

Author: uncleGen <[email protected]>

Closes apache#16949 from uncleGen/SPARK-16122.
Yunni pushed a commit to Yunni/spark that referenced this pull request Feb 27, 2017
## What changes were proposed in this pull request?

follow up pr of apache#16949.

## How was this patch tested?

jenkins

Author: uncleGen <[email protected]>

Closes apache#17033 from uncleGen/doc-restapi-environment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants